fix: only wait for changeset if the result is not empty #4296

Skarlso · 2023-10-04T11:58:08Z

When using Apply with a set of files that end up with an empty first change set, wait will timeout waiting for nothing.

Only wait if the changeSet isn't empty.

makkes · 2023-10-04T13:46:08Z

internal/utils/apply.go

@@ -76,8 +76,10 @@ func Apply(ctx context.Context, rcg genericclioptions.RESTClientGetter, opts *ru
 		changeSet.Append(cs.Entries)
 	}

-	if err := waitForSet(rcg, opts, changeSet); err != nil {
-		return "", err
+	if len(changeSet.Entries) > 0 {


I'm not sure if this shouldn't be rather be made part of ResourceManager#WaitForSet.

Hmmm I think if you want to wait you would want to wait for something and not wait for whatever even if there is nothing there. This makes more sense here readability wise.

Ultimately I don't care where this check lives. 😊

If I call ResourceManager#WaitForSet with an empty set and it doesn't immediately return (because there's nothing to wait for) this is a bug in that method IMHO.

Why would you call Wait if there is nothing to wait for? :)

That's like, I don't expect any visitors, but I open the door just in case. :D

A function that behaves indeterministically if called with certain input is a recipe for disaster. Be liberal in what you accept. But I really don't mind in this specific case so just leaving this here as a general remark.

I agree that we should look into the logic here and see where it hangs: https://github.com/fluxcd/pkg/blob/main/ssa/manager_wait.go But also the check in this PR is good, no need to instantiate a whole resource manager if there is nothing to wait for.

I can do both if that's okay with you two?!

stefanprodan · 2023-10-04T18:31:14Z

This looks Ok to me, but where did you encounter an empty changeset using the CLI?

Skarlso · 2023-10-04T18:53:28Z

@stefanprodan I was monkeying around. :D Also, we re-used a tiny part of the flux code in OCM, like this Apply for a bootstrap command for OCM. ( We gave credit. :).

And then I applied some manifests which didn't have type 1 objects in it and it hang.

For flux, it would it mean during install or bootstrapping there is a corrupted manifest in Github I guess.

stefanprodan · 2023-10-04T19:08:10Z

A corrupted manifest will not pass validation which happens way before you get a changeset.

Skarlso · 2023-10-04T19:12:00Z

Alright, so outside of me monkeying, if you think it's unlikely to occur, I'm fine with ignoring it. :)

stefanprodan · 2023-10-04T19:27:01Z

Your PR is Ok, thank you! I was really intrigued if you actually hit this issue with bootstrap, when I wrote that code I was convinced there is no possible way to get an empty changeset as the bootstrap logic would fail way before.

Skarlso · 2023-10-04T19:28:45Z

@stefanprodan Hah, neat. :) WDYT about what Max is saying? I'm okay to move it into the wait instead if you also think it's better suited there. :)

stefanprodan · 2023-10-04T19:29:39Z

When the changeset could be empty, then we have this check in place, here is kustomize-controller:
https://github.com/fluxcd/kustomize-controller/blob/768968d061f2da9bd933d260aabcb2c8a24131ee/internal/controller/kustomization_controller.go#L844-L847

stefanprodan

LGTM

Thanks @Skarlso

hiddeco · 2023-10-12T11:53:42Z

@Skarlso any chance you can rebase this quickly to maintain your signature? 😄

Signed-off-by: Gergely Brautigam <[email protected]>

Skarlso · 2023-10-12T11:55:06Z

done

fluxcdbot · 2023-10-12T12:12:24Z

Successfully created backport PR for release/v2.1.x:

[release/v2.1.x] fix: only wait for changeset if the result is not empty #4326

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [fluxcd/flux2](https://github.com/fluxcd/flux2) | Kustomization | patch | `v2.1.1` -> `v2.1.2` | --- > ⚠ **Warning** > > Some dependencies could not be looked up. Check the warning logs for more information. --- ### Release Notes <details> <summary>fluxcd/flux2 (fluxcd/flux2)</summary> ### [`v2.1.2`](https://github.com/fluxcd/flux2/releases/tag/v2.1.2) [Compare Source](fluxcd/flux2@v2.1.1...v2.1.2) #### Highlights Flux `v2.1.2` is a patch release which comes with various fixes. Users are encouraged to upgrade for the best experience. ##### Fixes - Ensures faster recovery of `Kustomization` and `HelmRelease` resources when the source-controller has restarted and is working on restoring the storage. - Prevent source-controller from failing to reconcile `OCIRepositories` when artifacts contain symlinks. - Addresses issue with helm-controller miss-labeling Custom Resource Definitions. - Detect immutable field errors in Google Cloud resources managed by Flux `Kustomizations`. - Better error reporting for `flux bootstrap` when the owner doesn't match the identity associated with the given token. - Allow `flux pull artifact` to fetch OCI artifacts produced by other tools. #### Components changelog - source-controller [v1.1.2](https://github.com/fluxcd/source-controller/blob/v1.1.2/CHANGELOG.md) - kustomize-controller [v1.1.1](https://github.com/fluxcd/kustomize-controller/blob/v1.1.1/CHANGELOG.md) - helm-controller [v0.36.2](https://github.com/fluxcd/helm-controller/blob/v0.36.2/CHANGELOG.md) #### CLI Changelog - PR [#4324](fluxcd/flux2#4324) - [@somtochiama](https://github.com/somtochiama) - bootstrap: Fix error msg when the Git token doesn't match the repo owner - PR [#4323](fluxcd/flux2#4323) - [@stefanprodan](https://github.com/stefanprodan) - e2e: Update Go dependencies - PR [#4313](fluxcd/flux2#4313) - [@fluxcdbot](https://github.com/fluxcdbot) - Update toolkit components - PR [#4296](fluxcd/flux2#4296) - [@Skarlso](https://github.com/Skarlso) - fix: only wait for changeset if the result is not empty - PR [#4285](fluxcd/flux2#4285) - [@matheuscscp](https://github.com/matheuscscp) - Add badge for SLSA Level 3 - PR [#4284](fluxcd/flux2#4284) - [@errordeveloper](https://github.com/errordeveloper) - Make `flux pull` work for OCI artifacts produced by other tools </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.home/nrdufour/home-ops/pulls/143 Co-authored-by: Renovate <[email protected]> Co-committed-by: Renovate <[email protected]>

Skarlso force-pushed the fix-apply-timeout-on-empty-set branch 2 times, most recently from 8bbadf1 to 3ecf95b Compare October 4, 2023 12:01

makkes reviewed Oct 4, 2023

View reviewed changes

stefanprodan approved these changes Oct 5, 2023

View reviewed changes

hiddeco added the backport:release/v2.1.x To be backported to release/v2.1.x label Oct 12, 2023

hiddeco approved these changes Oct 12, 2023

View reviewed changes

hiddeco force-pushed the fix-apply-timeout-on-empty-set branch from 3ecf95b to 2b62106 Compare October 12, 2023 11:52

fix: only wait for changeset if the result is not empty

a51ede6

Signed-off-by: Gergely Brautigam <[email protected]>

Skarlso force-pushed the fix-apply-timeout-on-empty-set branch from 2b62106 to a51ede6 Compare October 12, 2023 11:55

hiddeco merged commit 771b7ab into fluxcd:main Oct 12, 2023
7 checks passed

fluxcdbot mentioned this pull request Oct 12, 2023

[release/v2.1.x] fix: only wait for changeset if the result is not empty #4326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: only wait for changeset if the result is not empty #4296

fix: only wait for changeset if the result is not empty #4296

Skarlso commented Oct 4, 2023 •

edited

Loading

makkes Oct 4, 2023

Skarlso Oct 4, 2023

makkes Oct 4, 2023

Skarlso Oct 4, 2023

Skarlso Oct 4, 2023

makkes Oct 4, 2023

stefanprodan Oct 4, 2023

Skarlso Oct 5, 2023

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

stefanprodan left a comment

hiddeco commented Oct 12, 2023

Skarlso commented Oct 12, 2023

fluxcdbot commented Oct 12, 2023

fix: only wait for changeset if the result is not empty #4296

fix: only wait for changeset if the result is not empty #4296

Conversation

Skarlso commented Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

Skarlso commented Oct 4, 2023

stefanprodan commented Oct 4, 2023

stefanprodan left a comment

Choose a reason for hiding this comment

hiddeco commented Oct 12, 2023

Skarlso commented Oct 12, 2023

fluxcdbot commented Oct 12, 2023

Skarlso commented Oct 4, 2023 •

edited

Loading